There may be a better name for the session setting than error_on_remote_scan, such as error_on_remote_access. Suggestions welcome. If none, I may change it to error_on_remote_access.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @msbutler, and @rytaft)

OK, thanks. Phase 1 is only doing static checks, but does handle a few simple cases which could return Query is not running in its home region..

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @msbutler, and @rytaft)

Updated the session flag name to enforce_home_region and the modified the PR description.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @msbutler, and @rytaft)

While I agree that "home" may be the right terminology to use long-term, I worry about the impact that it will have on our docs short term. Even the introduction of the term here may require a fair bit of explanation.

What if we went with enforce_local_access instead? Local shouldn't need too much explanation, and it gets away from having to introduce a new concept just for this session variable?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @msbutler, and @rytaft)

Ahh, perfect. I didn't realize we were using it so extensively. I'm good with the enforce_home_region naming.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @msbutler, and @rytaft)

Sorry for the delay. This is looking great!

Reviewed 23 of 39 files at r1, 16 of 16 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, @msbutler, and @msirek)

-- commits line 10 at r2:
This is kind of misleading -- the PRIMARY region is a different concept from home region (although they are related...)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 30 at r2 (raw file):

    message_id   UUID DEFAULT gen_random_uuid(),
    message    STRING NOT NULL,
    crdb_region crdb_internal_region NOT NULL,

what is the benefit of including this column here? Seems like this would add confusion

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 40 at r2 (raw file):

    message_id   UUID DEFAULT gen_random_uuid(),
    message    STRING NOT NULL,
    crdb_region crdb_internal_region NOT NULL,

ditto

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 139 at r2 (raw file):

# Non-locality-optimized lookup join should error out.
statement error pq: Query has no home region. Try removing the join expression.

"Try removing the join expression" seems like it would not be desirable for the user. Presumably they wanted to use a join. Maybe this should suggest something like "Try adding a filter on c_id and/or crdb_region"

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 238 at r2 (raw file):

# Select from REGIONAL BY TABLE should indicate the gateway region to use.
statement error pq: Query is not running in its home region. Try running the query from region 'ap-southeast-2'.

Shouldn't this error say "try running the query from region 'us-east-1'"?

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 332 at r2 (raw file):

# Statements which previously succeeded should now fail under REGION survivability.
statement error pq: Query has no home region. Try accessing only tables in multi-region databases with ZONE survivability.

Technically the query still has a home region, so I think this error is misleading. I'd just say something like "enforce_home_region setting cannot be combined with REGION survivability"

pkg/sql/vars.go line 2123 at r2 (raw file):

		},
	},
	// CockroachDB extension.

nit: add blank line above

pkg/sql/opt/distribution/distribution.go line 68 at r2 (raw file):

// by a non-multiregion database or a multiregion database with SURVIVE REGION
// FAILURE goal.
func errorOnInvalidMultiregionDB(evalCtx *eval.Context, tabMeta *opt.TableMeta) {

Feels like this check should go in buildScan in the optbuilder.

pkg/sql/opt/memo/expr.go line 302 at r2 (raw file):

		}
	}
	return ""

would it be better to have two return params, with ok bool to indicate if it was possible to get a single region?

pkg/sql/opt/xform/coster.go line 162 at r2 (raw file):

	// which are not locality-optimized or Distribute operations when a session
	// mode is set to error out on remote node accesses.
	remoteCostPenalty = 1e8

Why don't we just use hugeCost? It's designed for cases like this.

pkg/sql/opt/xform/coster.go line 675 at r2 (raw file):

		// the cost of using other indexes to `hugeCost`, so let's make the
		// current plan's costs expensive, but not on par with the illegal index
		// plan since we never want the plan with the illegal index to win out.

This seems overly complicated for little benefit -- I'd stick with hugeCost

pkg/sql/opt/xform/optimizer.go line 792 at r2 (raw file):

				// setLowestCostTree does this walking already, so this check is
				// included here to avoid an extra walking step.
				if !join.LocalityOptimized {

Can this check be done in the execbuilder? That's where we typically put these kinds of checks.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, and @msirek)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 139 at r2 (raw file):

# Non-locality-optimized lookup join should error out.
statement error pq: Query has no home region. Try removing the join expression.

We should maybe file an issue or add a TODO somewhere to add hints to errors that point to docs with more information about what these errors mean.

Thanks.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, and @rytaft)

-- commits line 10 at r2:

Removed the reference to primary region:

A home region specifies the region(s) from which consistent reads from a
set of rows in a table can be served locally.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 30 at r2 (raw file):

Removed the column

Code quote:

 crdb_region crdb_internal_region NOT NULL,

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 40 at r2 (raw file):

Removed the column

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 139 at r2 (raw file):

OK. I guess we'd want to handle regional by row tables on either side of the lookup join. Updated the error in this case to:

Query has no home region. Try adding a filter on child.crdb_region and/or on key column (child.c_id). Try adding a filter on parent.crdb_region and/or on key column (parent.p_id).

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 139 at r2 (raw file):

OK. I've created #86178. Perhaps you could update the issue with more details, or your vision here, if there are specific things you'd like to see. Also, I'm not sure if we need to engage someone else to get new sections added to docs. Maybe @rytaft could help coordinate this.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 238 at r2 (raw file):

Fixed, thanks.

Code quote:

 Select from REGIONAL BY TABLE should indicate the gateway region to use.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 332 at r2 (raw file):

Fixed

pkg/sql/vars.go line 2123 at r2 (raw file):

Done

Code quote:

`enforce_home_region`: {

pkg/sql/opt/distribution/distribution.go line 68 at r2 (raw file):

That's a possibility, but if we do the check before normalization, then if the scan is not actually used, like when we have an unsatisfiable filter:
EXPLAIN SELECT * FROM messages WHERE account_id = 1 AND account_id = 2
... we will end up unnecessarily erroring out. Thoughts?

pkg/sql/opt/memo/expr.go line 302 at r2 (raw file):

Modified as suggested

Code quote:

func (e *DistributeExpr) GetRegionOfDistribution()

pkg/sql/opt/xform/coster.go line 162 at r2 (raw file):

Returning hugeCost may cause the cost of a plan with a forced index (hinted index) to be more expensive than a plan without the forced index, since hinting uses this same mechanism. Then, when the optimizer sees an index picked which is not the hinted index, the query errors out, saying that an illegal index was chosen (I actually ran into this issue). So, if we want to use hugeCost here, we'd need a different mechanism to enforce selection of the hinted index.

pkg/sql/opt/xform/coster.go line 675 at r2 (raw file):

See comment above.

pkg/sql/opt/xform/optimizer.go line 792 at r2 (raw file):

OK, done. This changes the behavior so we don't error out EXPLAIN. I was previously wondering if we wanted to allow EXPLAIN to run, even if the actual query errors out. It seems like we would, to allow the user to analyze the chosen query plan and maybe get a better sense of how to correct the query.

Code quote:

TODO(msirek): Remove this in phase 2 or 3 when we can dynamical

Sorry, published my other changes too early -- just added a few additional comments

Reviewed 4 of 38 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @msirek, and @taroface)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 160 at r5 (raw file):

# Locality optimized lookup join or lookup join that might access multiple
# regions is not allowed in phase 1.

I think this should be allowed.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 492 at r5 (raw file):

SELECT * FROM non_multiregion_test_db.messages, multi_region_test_db.messages_global

# Scans from ZONE survivable tables with contradictions in predicates are not allowed.

do you mean non-multiregion?

pkg/sql/opt/cat/table.go line 168 at r5 (raw file):

(it's also fine to leave as-is if this is more effort for some reason)

pkg/sql/opt/distribution/distribution.go line 51 at r5 (raw file):

		// operation to be applied on the input to the lookup join instead of on
		// the lookup join itself.
		provided = BuildLookupJoinLookupTableDistribution(t.Memo().EvalContext(), t, t.Memo().RootIsExplain())

Sorry, I realize I might have led you astray before. This lookup join lookup table distribution is going to be great for costing purposes, but it should not be used to determine the output distribution of the lookup join. The output distribution of a lookup join should be the same as its input distribution (think about a DistSQL diagram with a join reader). So it's awesome that you wrote the BuildLookupJoinLookupTableDistribution function -- but I think it should only be called from the coster, not here.

pkg/sql/opt/distribution/distribution.go line 114 at r5 (raw file):

This will probably change anyway based on my comment above...

pkg/sql/opt/exec/execbuilder/relational.go line 1847 at r5 (raw file):

I meant nit: false /* ... */, true /* ... */

pkg/sql/opt/xform/coster.go line 948 at r4 (raw file):

(ditto for inverted join below)

Thanks. No worries.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @rytaft, and @taroface)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 160 at r5 (raw file):

OK. I misinterpreted your earlier comment about not checking for locality-optimized join.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 492 at r5 (raw file):

Fixed, thanks.

pkg/sql/opt/metadata.go line 571 at r5 (raw file):

Just for ease of use. For example, if a query contains 20 tables, specifying the alias is the message points the user directly to the table in question instead of making them spend time examining all of their tables to find the one with the referenced columns.

Code quote:

If there's another column in the metadata with the same column alias but

pkg/sql/opt/cat/table.go line 168 at r5 (raw file):

No reason, just following what was present in the data structure. Modified it to return the home region column name in all cases. Now this method can also be used to detect if a table is REGIONAL BY ROW.

Code quote:

RegionalByRowAsColNameOverride

pkg/sql/opt/distribution/distribution.go line 51 at r5 (raw file):

No worries. Applied it only to costing now. I've added in code to set the distribution of locality optimized join to the gateway region to avoid distribute ops and extra added cost. Without this code we don't pick locality-optimized join sometimes. What do you think about this approach?

pkg/sql/opt/distribution/distribution.go line 114 at r5 (raw file):

Yes, this is to try and fix the overloaded meaning of an empty distribution. I've added a TODO here and opened issue #86641.

pkg/sql/opt/distribution/distribution.go line 127 at r5 (raw file):

Code is removed. Added a TODO here and opened issue #86641.

pkg/sql/opt/distribution/distribution.go line 135 at r5 (raw file):

I was thinking the absence of a relational expression (for example, VALUES, scalar function call, etc.), could be executed locally, so that's the condition in the code I added instead of calling out all possible scalar expressions. Removed this; to be handled through #86641.

pkg/sql/opt/distribution/distribution.go line 171 at r5 (raw file):

It seems like it. addEqualityColumns appends to keyCols or lookupExpr in the same order as index columns are defined:

... and this is later copied into LookupJoinPrivate.LookupExpr.

Code quote:

firstIndexColEqExpr := lookupJoin.LookupJoinPrivate.LookupExpr[0].Condition

pkg/sql/opt/exec/execbuilder/relational.go line 1912 at r3 (raw file):

No worries. Modified the code to use the lookup distribution for lookup join costing. Other joins will incur the largeDistributeCost from the distribute operation.

pkg/sql/opt/exec/execbuilder/relational.go line 1847 at r5 (raw file):

Fixed

Code quote:

colName := b.mem.Metadata().QualifiedAlias(colID, false, true, b.catalog)

pkg/sql/opt/exec/execbuilder/relational.go line 1992 at r5 (raw file):

Fixed

pkg/sql/opt/memo/expr_format.go line 1427 at r5 (raw file):

Fixed

pkg/sql/opt/memo/memo.go line 508 at r5 (raw file):

I think that might have been a result of the code added to modify the distribution of lookup join. Skipping over DistributeOp is no longer needed and is removed.

pkg/sql/opt/props/physical/distribution.go line 117 at r5 (raw file):

When error checking for lookup join was moved to the execbuilder, it caused EXPLAIN statements to not error out. To maintain consistency with those errors (and also to allow EXPLAIN to be run to help the user see the source of the locality issues), all erroring is now avoided for EXPLAIN.

Code quote (from pkg/sql/opt/memo/expr_format.go):

md.QualifiedAlias(

pkg/sql/opt/props/physical/distribution.go line 198 at r5 (raw file):

OK, I guess that terminology doesn't apply to tables (e.g. the REGIONAL BY TABLE primary region), just databases. Updated the comment to use "home region".

Code quote:

Use the leaseholder region(s), which should be the same as the PRIMARY

pkg/sql/opt/xform/coster.go line 948 at r4 (raw file):

Re-added it. We can't know at this point what the distribution of the Input is, so adding a large cost if not locality optimized might miss some local regular lookup joins. I think the distribute op and added costs would take care of the locality optimized case. I don't follow the comment about inverted join. Details?

pkg/sql/opt/xform/testdata/rules/join line 10739 at r5 (raw file):

Fixed

Code quote:

SELECT * FROM def_part WHERE NOT EXISTS (SELECT * FROM abc_part WHERE e = a) AND d = 1

Reviewed 15 of 15 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @msirek, and @taroface)

pkg/sql/opt/cat/table.go line 169 at r6 (raw file):

	// REGIONAL BY ROW TABLE, otherwise "", false is returned.
	// This column is name `crdb_region` by default, but may be overridden with a
	// different name in a `REGIONAL BY ROW AS` DDL clause.

Do you have any tests with REGIONAL BY ROW AS?

pkg/sql/opt/distribution/distribution.go line 51 at r5 (raw file):

Seems fine for now, since I think this is the same assumption that the exploration rules generating locality optimized lookup joins make. We can revisit later if need be.

pkg/sql/opt/distribution/distribution.go line 171 at r5 (raw file):

Is there a way we can ensure that doesn't change? Maybe add a comment in addEqualityColumns and/or add a test case that will fail if it changes?

pkg/sql/opt/distribution/distribution.go line 120 at r6 (raw file):

		if localityOptimizedJoin {
			provided.FromLocality(evalCtx.Locality)
		}

nit: You could just move this up to where you check for a LookupJoinExpr and break from the switch

pkg/sql/opt/exec/execbuilder/relational.go line 763 at r6 (raw file):

	if b.evalCtx.SessionData().EnforceHomeRegion && !b.mem.RootIsExplain() && !b.skipScanExprCollection {
		if b.builtScans == nil {
			b.builtScans = make([]*memo.ScanExpr, 0, 2)

Why cap=2?

pkg/sql/opt/exec/execbuilder/relational.go line 1750 at r6 (raw file):

		return errors.AssertionFailedf("The gateway region could not be determined while enforcing query home region.")
	}
	for i, scan := range b.builtScans {

Instead of this mechanism, it seems like it might be better to take advantage of the fact that you've set expressions to have hugeCost in the coster, and only call this function if you find that a DistributeExpr has hugeCost. At that point, you know you'll be throwing an error anyway, so it's ok to traverse the expression tree again to pick out the scans. I'd rather not add more unnecessary bookkeeping to the happy path.

pkg/sql/opt/exec/execbuilder/relational.go line 2076 at r6 (raw file):

		if rel.Op() == opt.ScanOp {
			b.skipScanExprCollection = true
		}

I think you can remove this code if you take my suggestion above

pkg/sql/opt/xform/coster.go line 948 at r4 (raw file):

Inverted join is similar to lookup join in that it performs lookups into an index using the kv api (the index just happens to be inverted)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @rytaft, and @taroface)

pkg/sql/opt/cat/table.go line 169 at r6 (raw file):

Yes, here:
https://github.com/msirek/cockroach/blob/d88c22003dac4df02eb11b3e6907a8d2070d97ef/pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error#L377-L381

pkg/sql/opt/distribution/distribution.go line 51 at r5 (raw file):

OK, thanks.

pkg/sql/opt/distribution/distribution.go line 171 at r5 (raw file):

I found method getConstPrefixFilter which ensures that the filter is on the first index column. I moved this to be a method of LookupJoinPrivate and am using it now.

Code quote:

lookupJoin.LookupJoinPrivate.LookupExpr

pkg/sql/opt/distribution/distribution.go line 120 at r6 (raw file):

done

Code quote:

// Locality-optimized join is considered to be local.

pkg/sql/opt/exec/execbuilder/relational.go line 763 at r6 (raw file):

Large enough to handle simple 2-table join queries without wasting memory.

pkg/sql/opt/exec/execbuilder/relational.go line 1750 at r6 (raw file):

OK, changed skipScanExprCollection to doScanExprCollection so by default we don't collect the Scans. If the Distribute hashugeCost, we set doScanExprCollectionto true and traverse the tree again to populate thebuiltScansslice before callingenforceScanWithHomeRegion. BTW, I added code to ensure a DistributeExpris never built with anExplainExpr` as its input.

pkg/sql/opt/exec/execbuilder/relational.go line 2076 at r6 (raw file):

We still need this code because the lookup join may be locality optimized, in which case the Scan needs to be ignored.

Code quote:

enforceScanWithHomeRegion

pkg/sql/opt/xform/coster.go line 948 at r4 (raw file):

Thanks, added handling for inverted join, plus inverted join as input to a lookup join.

Reviewed 8 of 9 files at r7, 10 of 10 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @msirek, and @taroface)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 599 at r8 (raw file):

# Inverted join doing lookup into a REGIONAL BY ROW table is not allowed.
statement error pq:

nit: this is a regex so you should be able to include some part of the error message

pkg/sql/opt/distribution/distribution.go line 149 at r8 (raw file):

	} else if lookupTable.IsRegionalByRow() {
		if len(lookupJoin.KeyCols) > 0 {
			projectExpr := lookupJoin.Input

nit: rename variable projectExpr since it's no longer necessarily a project

pkg/sql/opt/distribution/distribution.go line 152 at r8 (raw file):

			firstKeyColID := lookupJoin.LookupJoinPrivate.KeyCols[0]
			if projectExpr.Op() == opt.InvertedJoinOp {
				invertedJoinExpr, _ := projectExpr.(*memo.InvertedJoinExpr)

nit: instead of checking the op, you can just do

if invertedJoinExpr, ok := projectExpr.(*memo.InvertedJoinExpr); ok {

here and below with constExpr

pkg/sql/opt/distribution/distribution.go line 163 at r8 (raw file):

					}
				}
			} else if projection, ok := projectExpr.(*memo.ProjectExpr); ok {

nit: rename projection to projectExpr

pkg/sql/opt/distribution/distribution.go line 195 at r8 (raw file):

// statically determined.
func BuildInvertedJoinLookupTableDistribution(
	evalCtx *eval.Context, invertedJoin *memo.InvertedJoinExpr, forExplain bool,

I'm still pretty negative about this special forExplain param you've added everywhere and the checks for RootIsExplain. What happens if you remove them all?

pkg/sql/opt/exec/execbuilder/relational.go line 763 at r6 (raw file):

Consider adding a comment to that effect

pkg/sql/opt/exec/execbuilder/relational.go line 2076 at r6 (raw file):

If the lookup join is locality optimized, the input should also have a local distribution. If that's not the case today, maybe add a TODO to get rid of this.

pkg/sql/opt/exec/execbuilder/relational.go line 1805 at r8 (raw file):

	if b.evalCtx.SessionData().EnforceHomeRegion && !b.mem.RootIsExplain() {
		if distribute.Cost() >= xform.LargeDistributeCost {

This cost check isn't needed since EnforceHomeRegion automatically means that any no-op distribution will cause an error.

pkg/sql/opt/exec/execbuilder/relational.go line 2038 at r8 (raw file):

						}
					}
				}

Looks like all this code is basically duplicated from distribution.go. Is there any way to refactor a bit to avoid the duplication? (also, I have all the same nits here as I had in that file)

pkg/sql/opt/exec/execbuilder/relational.go line 2093 at r8 (raw file):

		}
	}
	saveDoScanExprCollection := b.doScanExprCollection

this should be inside the enforceHomeRegion block, since it's inside the block below.

pkg/sql/opt/exec/execbuilder/relational.go line 2114 at r8 (raw file):

		//               determine if the lookup will be local, or after #69617 is
		//               merged.
		err = b.handleRemoteLookupJoinError(join)

I think you should check for LargeDistributeCost before calling this.

pkg/sql/opt/exec/execbuilder/relational.go line 2312 at r8 (raw file):

func (b *Builder) buildInvertedJoin(join *memo.InvertedJoinExpr) (execPlan, error) {
	enforceHomeRegion := b.evalCtx.SessionData().EnforceHomeRegion && !b.mem.RootIsExplain()
	saveDoScanExprCollection := b.doScanExprCollection

this should be inside the enforceHomeRegion block, since it's inside the block below.

pkg/sql/opt/exec/execbuilder/relational.go line 2320 at r8 (raw file):

		}
		if rel.Op() == opt.ScanOp {
			b.doScanExprCollection = true

Why are you setting this to true if we're not in an error case?

Also, we don't support locality optimized inverted joins, so I think you can just remove this.

pkg/sql/opt/exec/execbuilder/relational.go line 2333 at r8 (raw file):

		//               determine if the lookup will be local, or after #69617 is
		//               merged.
		err = b.handleRemoteInvertedJoinError(join)

I think you should check for LargeDistributeCost before calling this.

pkg/sql/opt/memo/expr.go line 812 at r8 (raw file):

		if filter.Condition.Op() == opt.EqOp {
			leftChild := filter.Condition.Child(0)
			if leftChild.Op() == opt.VariableOp {

nit: here you can do:
if variableExpr, ok := leftChild.(*VariableExpr); ok {

pkg/sql/opt/memo/expr.go line 820 at r8 (raw file):

		}
	}
	return nil /* expr */, false /* ok */

nit: we don't usually include comments like this for return params

pkg/sql/opt/memo/expr.go line 834 at r8 (raw file):

// constant values. If such a filter is found, GetConstPrefixFilter returns the
// position of the filter and ok=true. Otherwise, returns ok=false.
func (lj *LookupJoinPrivate) GetConstPrefixFilter(memo *Memo) (pos int, ok bool) {

nit: doesn't seem like you need the whole memo -- why not just pass what you need? Maybe just metadata?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @rytaft, and @taroface)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 599 at r8 (raw file):

Fixed

pkg/sql/opt/distribution/distribution.go line 149 at r8 (raw file):

Done

Code quote:

projectExpr := lookupJoin.Input

pkg/sql/opt/distribution/distribution.go line 152 at r8 (raw file):

Done

Code quote:

== opt.InvertedJoinOp

pkg/sql/opt/distribution/distribution.go line 163 at r8 (raw file):

Done

Code quote:

projection, ok := projectExpr.(*memo.ProjectExpr); ok

pkg/sql/opt/distribution/distribution.go line 195 at r8 (raw file):

Removed them. Since most checks are in the execbuilder there may not be a large impact, but possibly there will be some cases where we'd be unable to get an explain with the session flag on.

pkg/sql/opt/exec/execbuilder/relational.go line 763 at r6 (raw file):

Done

pkg/sql/opt/exec/execbuilder/relational.go line 2076 at r6 (raw file):

One problem case is when we have a lookup join with each side of the join in a different region, we want to make sure the Query has no home region error is returned. Removing this code causes the error Query is not running in its home region message to be returned because the lookup table is not represented as a Scan. Also, if by collecting all of the Scans, even those under locality-optimized joins, an RBR table input to a locality optimized join might not have a single home region, so we don't want that Scan to be processed and cause the query to error out.

pkg/sql/opt/exec/execbuilder/relational.go line 1805 at r8 (raw file):

Removed

Code quote:

if distribute.Cost() >= xform.LargeDistributeCost {

pkg/sql/opt/exec/execbuilder/relational.go line 2038 at r8 (raw file):

Created function GetDEnumAsStringFromConstantExpr for this.

Code quote:

saveDoScanExprCollection := b.doScanExprCollection

pkg/sql/opt/exec/execbuilder/relational.go line 2093 at r8 (raw file):

Moved

pkg/sql/opt/exec/execbuilder/relational.go line 2114 at r8 (raw file):

That could only be done if the distribution of the Input to the lookup join or inverted join were known at costing time. Since it's not, we might not add the LargeDistributeCost in the coster, but by the time the distributions are all filled in, we have a join in hand which needs to error out.

Code quote:

TODO(msirek): Update this in phase 

pkg/sql/opt/exec/execbuilder/relational.go line 2312 at r8 (raw file):

Moved it

pkg/sql/opt/exec/execbuilder/relational.go line 2320 at r8 (raw file):

Corrected this to set it to false. It's actually an issue with returning the correct error message on mismatched left and right inputs (see previous comment).

pkg/sql/opt/exec/execbuilder/relational.go line 2333 at r8 (raw file):

See previous comment. The coster is not aware of the Input distribution. Also, the Distribute operation may be the one recording the LargeDistributeCost, and it may be sitting above the join and not be accessible here if the Input itself it local, but the join as a whole, is not.

Code quote:

b.handleRemoteInvertedJoinError(join)

pkg/sql/opt/memo/expr.go line 812 at r8 (raw file):

Done

Code quote:

func (ij *InvertedJoinPrivate) GetConstExprFromFilter(colID opt.ColumnID) (expr opt.Expr, ok bool) {

pkg/sql/opt/memo/expr.go line 820 at r8 (raw file):

Removed

pkg/sql/opt/memo/expr.go line 834 at r8 (raw file):

Done

thanks for working through all these details

Reviewed 19 of 19 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @msbutler, @msirek, and @taroface)

pkg/sql/opt/distribution/distribution.go line 195 at r8 (raw file):

Thank you!

pkg/sql/opt/exec/execbuilder/relational.go line 2012 at r9 (raw file):

		if lookupTable.IsRegionalByRow() {
			if len(join.KeyCols) > 0 {
				projectExpr := join.Input

nit: projectExpr -> inputExpr

pkg/sql/opt/exec/execbuilder/relational.go line 2014 at r9 (raw file):

				projectExpr := join.Input
				firstKeyColID := join.KeyCols[0]
				if projectExpr.Op() == opt.InvertedJoinOp {

nit: if invertedJoinExpr, ok := project.(*memo.InvertedJoinExpr); ok {

pkg/sql/opt/exec/execbuilder/relational.go line 2021 at r9 (raw file):

						}
					}
				} else if projection, ok := projectExpr.(*memo.ProjectExpr); ok {

nit: projection -> projectExpr

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @msbutler, @rytaft, and @taroface)

pkg/sql/opt/exec/execbuilder/relational.go line 2012 at r9 (raw file):

Done

Code quote:

projectExpr := join.Input

pkg/sql/opt/exec/execbuilder/relational.go line 2014 at r9 (raw file):

Done

pkg/sql/opt/exec/execbuilder/relational.go line 2021 at r9 (raw file):

Done

Code quote:

projection, ok := projectExpr

TFTR!
bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @msbutler, @rytaft, and @taroface)

Reviewed 3 of 5 files at r10, 12 of 12 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 173 at r11 (raw file):

# Zone configs sometimes are not available right away. Issue a couple queries
# that take a while to compile to make sure they're available.

FYI, there is a sleep directive in logictests